Skip to content

ci: keep build timing report non-blocking when PR comment fails - #592

Merged
quangvdao merged 2 commits into
Verified-zkEVM:mainfrom
ainta:ci/timing-report-comment-nonblocking
Jul 23, 2026
Merged

ci: keep build timing report non-blocking when PR comment fails#592
quangvdao merged 2 commits into
Verified-zkEVM:mainfrom
ainta:ci/timing-report-comment-nonblocking

Conversation

@ainta

@ainta ainta commented Jun 26, 2026

Copy link
Copy Markdown
Contributor

Summary

  • Grant the Build Timing Report report job the token permissions it actually uses: actions: read, contents: read, and pull-requests: write.
  • Remove the workflow-level issues: write / pull-requests: read block, and scope the requested permissions to the reporting job.
  • Keep the PR comment upsert resilient for expected GitHub comment API failures by logging the HTTP status/message and leaving the rendered report in the job summary.
  • Let unexpected JavaScript errors and unexpected API statuses fail the job so real regressions stay visible.

Context

The report renderer succeeds, but the final comment-upsert step can fail with:

POST /repos/Verified-zkEVM/ArkLib/issues/<n>/comments - 403
##[error]Unhandled error: HttpError: Resource not accessible by integration

#632 identified the permission side of the same failure: for comments on pull requests, the workflow should request pull-requests: write. This PR now folds that permission correction into the existing resilience work, while keeping the rendered timing report available through $GITHUB_STEP_SUMMARY if GitHub still rejects the comment API call.

The updated diagnosis is that the workflow should request the correct job-level GITHUB_TOKEN scope directly. If an organization/repository setting or rate limit still prevents the comment operation, the workflow emits a warning containing HTTP <status>: <message> and does not turn an otherwise-green CI run red for the cosmetic comment.

Verification

  • python3 -c "import yaml; yaml.safe_load(open('.github/workflows/build-timing-report.yml'))"
  • git diff --check
  • Extracted the Upsert build timing PR comment github-script body and syntax-checked it with bundled Node after wrapping it in an async function.

Posted by Codex on behalf of the user (@ainta) with approval.

The "Upsert build timing PR comment" step throws on any GitHub API error,
which fails the whole Build Timing Report job. In practice the comment call
returns HTTP 403 "Resource not accessible by integration": this is a
`workflow_run` job, so GITHUB_TOKEN is capped at the repository's default
token permissions and the `issues: write` declared in the workflow cannot
take effect. The rendered report is already attached to the job summary, so
the comment is cosmetic, yet the failure shows up as a red X on green CI.

Wrap the comment upsert in try/catch: on failure emit a core.warning (with a
pointer to the Actions workflow-permission setting on 403) and let the job
finish green. No behavior change when the comment posts successfully.

Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
@github-actions

github-actions Bot commented Jun 26, 2026

Copy link
Copy Markdown
Contributor

🤖 PR Summary

This PR contains a single change: it adjusts the GitHub Actions workflow for the build timing report (build-timing-report.yml) by moving the permissions block from the workflow-level scope into the report job scope. It also corrects the pull-requests permission from read to write and removes the unused issues: write permission. In addition, it introduces a warnExpectedCommentApiFailure helper to catch specific HTTP errors (403, 404, 410, 422, 429) on comment API calls, logging a warning and returning early instead of failing the job. Unexpected errors are still rethrown. No new sorries or admits are introduced.


Statistics

Metric Count
📝 Files Changed 1
Lines Added 55
Lines Removed 22

Lean Declarations

  • No declarations were added, removed, or affected.

sorry Tracking

  • No sorrys were added, removed, or affected.

📋 **Additional Analysis**

No findings.


📄 **Per-File Summaries**
  • .github/workflows/build-timing-report.yml: Moved the permissions block from the workflow top-level into the report job, changing pull-requests from read to write and dropping the issues: write permission (which was unused). Added a warnExpectedCommentApiFailure helper that catches HTTP 403, 404, 410, 422, and 429 errors on the listComments, updateComment, and createComment API calls, logs a warning (with a permission hint on 403), and returns early instead of failing the workflow; unexpected errors are still rethrown. This prevents a transient API permission issue from blocking the whole build-timing report while still warning about the failure.

Last updated: 2026-07-22 23:31 UTC.

@quangvdao

Copy link
Copy Markdown
Collaborator

Thanks for diagnosing the noisy failure. I’d like to retain the PR comment, so please revise this PR to incorporate the permission correction while keeping the reporting step resilient:

  • change pull-requests: read to pull-requests: write;
  • remove issues: write so the workflow has only the write scope it actually uses;
  • preferably scope these permissions to the report job;
  • keep expected comment API failures non-blocking and emit the HTTP status/message;
  • do not catch every exception indiscriminately—unexpected JavaScript/programming errors should still fail so genuine regressions remain visible;
  • update the PR explanation: GitHub permits workflow-level permissions to override the repository’s restricted default, so the current “default token cap” diagnosis is not quite right.

I’ve closed #632 in favor of consolidating both fixes here.

@ainta

ainta commented Jul 22, 2026

Copy link
Copy Markdown
Contributor Author

Addressed in c60e387.

Changes made:

  • moved the workflow permissions down to the report job;
  • changed the PR permission to pull-requests: write and removed issues: write;
  • narrowed the non-blocking handling to expected comment API failures, with HTTP <status>: <message> in the warning;
  • left unexpected JavaScript errors and unexpected API statuses to fail the job;
  • updated the PR body to remove the earlier repository-default-token-cap diagnosis and reflect the consolidated fix(ci): Build Timing Report cannot post its PR comment (pull-requests: write) #632 permission fix.

Local checks run:

  • python3 -c "import yaml; yaml.safe_load(open(\".github/workflows/build-timing-report.yml\"))"
  • git diff --check
  • extracted the github-script body and syntax-checked it with bundled Node after wrapping it in an async function.

The lightweight PR checks have passed; the full build/docs jobs are still in progress.

@quangvdao
quangvdao merged commit 58a5cf1 into Verified-zkEVM:main Jul 23, 2026
4 of 6 checks passed
katyhr pushed a commit that referenced this pull request Jul 26, 2026
Grant the Build Timing Report job the permissions it uses: `actions: read`, `contents: read`, and `pull-requests: write`. Scope them to the `report` job and remove the redundant `issues` permission.

Keep expected PR comment API failures (HTTP 403, 404, 410, 422, and 429) non-blocking while recording the status and message and preserving the rendered report in the job summary. Unexpected JavaScript errors and unexpected HTTP statuses still fail.

Consolidates the permission fix from #632.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants